Fix path traversal via metadata file_name in folder-based builders by Kaif10 · Pull Request #8325 · huggingface/datasets

Kaif10 · 2026-07-12T23:48:39Z

Vulnerability

The folder-based builders (imagefolder, audiofolder, videofolder, pdffolder) resolve each metadata row's file_name by joining it to the metadata file's directory with no containment check. A malicious dataset can set file_name to a ../-traversal path or an absolute path, causing the builder to open and read files outside the dataset directory when the dataset is loaded — arbitrary file read (CWE-22).

Fix

FolderBasedBuilder._generate_examples now rejects, with a ValueError naming the offending file_name, any value that is absolute or uses .. traversal to escape the directory containing the metadata file. The validation is performed on the relative reference so it applies uniformly to local dataset directories and to the fsspec URLs used when reading from downloaded archives; os.path.realpath / os.path.commonpath were deliberately not used on the joined path because those functions are not fsspec-aware (unlike the streaming-patched os.path.join) and would corrupt archive URLs such as zip://...::.... For genuine local paths, symlinks are additionally resolved and containment is verified with os.path.commonpath. The containment root is the metadata file's own directory, matching the existing resolution base and preserving all legitimate relative references into subdirectories.

Tests

Added regression tests in tests/packaged_modules/test_folder_based_builder.py: ../ traversal rejected (three variants, including subdir/../../ forms), absolute path rejected, and legitimate same-dir / subdir/ / ./subdir/ references still load. These fail on the pre-fix code and pass with the fix. test_folder_based_builder.py (51 passed) and test_imagefolder.py (36 passed).

Follows the containment approach of #8303.

AI assistance (Claude) was used in preparing this change; it was reviewed and verified locally.

…uggingface#8324) The `file_name` field from a dataset's metadata.jsonl/metadata.csv/metadata.parquet was normalized and joined to the metadata file's directory without any containment check. A crafted `file_name` such as "../../etc/passwd" or an absolute path escaped the dataset directory, letting a malicious dataset read arbitrary files on the host when loaded with imagefolder/audiofolder/videofolder/pdffolder (CWE-22). Reject any `file_name` that is absolute or uses ".." traversal to escape the directory containing the metadata file. The check is done on the relative reference so it works for both local directories and the fsspec URLs used for downloaded archives; for local paths it additionally resolves symlinks and verifies containment with os.path.commonpath. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lhoestq

Thanks for reporting and for the fix, I just have one comment:

lhoestq · 2026-07-14T09:49:29Z

+                            # For local paths, additionally resolve symlinks and confirm containment.
+                            # Skipped for fsspec URLs (which contain "://") since realpath is not
+                            # aware of them and would corrupt the URL.
+                            if "://" not in item:


you can point to local files using file://../.. or local://../.. so this check is maybe not enough

maybe we need to forbid :// altogether ? (except for zip://<file_name>::<valid_relative_path>)

You're right — file:// and local:// (and any other fsspec scheme) resolve to local files and slip past the containment check, since the old guard skipped it whenever file_name contained ://. On Linux the scheme also survives normpath, so this is a real bypass.

I've switched to forbidding :// in the file_name value entirely, as you suggested. Legitimate archive loads are unaffected: their URL is zip://<file_name>::<container>, and the scheme is always on the container (downloaded_metadata_dir), never on the file_name itself — so I now key the realpath containment check on the metadata directory rather than the joined path. Added regression tests for file:// / local:// payloads and confirmed the existing zip-archive tests (streaming and non-streaming) still pass.

(Disclosure: I used AI assistance while preparing this change.)

huggingface#8324) Addresses lhoestq's review on huggingface#8325: the `"://" not in item` guard let a crafted `file_name` such as `file://../..` or `local://../..` skip the realpath containment check, since these fsspec schemes resolve to local files. Per the maintainer's suggestion, forbid any URL scheme ("://") in the attacker-controlled `file_name`; legitimate archive reads are unaffected because their `zip://<file_name>::<container>` URL carries the scheme on the metadata directory (container), never on `file_name`. The containment check is now keyed on the metadata directory rather than the joined item. Adds regression tests for file:// and local:// scheme payloads and keeps the existing traversal/absolute/legit-subdir and zip-archive tests passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

lhoestq

Thanks ! I added a few more comments

lhoestq · 2026-07-15T12:36:02Z

+                                or file_relpath == ".."
+                                or file_relpath.startswith("../")


if the path is like dummy/../../../ it can still escape

lhoestq · 2026-07-15T12:40:24Z

+                            if "://" not in downloaded_metadata_dir:
+                                real_root = os.path.realpath(downloaded_metadata_dir)
+                                real_path = os.path.realpath(item)


maybe you can have a check on both a chained url/path zip://...::... and a single url/path if you split downloaded_metadata_dir and item on :: to get only the first part before passing to commonpath() ?

we will need to make sure this works for these schemes:

/path/to/local/metadata_dir

hf://path/to/remote/metadata_dir

zip://metadata_dir::/path/to/local

zip://metadata_dir::hf://path/to/remote

…uggingface#8324) Follow-up to lhoestq's second review on huggingface#8325. 1. `dummy/../../../` style references (a valid segment followed by an escape) are already normalized by `os.path.normpath` and rejected by the existing "../" guard; add regression tests to lock this in. 2. The containment check used to be skipped whenever the metadata directory was an fsspec URL. Make it run for every form by splitting both the metadata dir and the joined reference on the "::" hop separator and validating the first component (the part a relative `file_name` can influence), while requiring the trailing container component(s) to stay identical. URL components are compared with posixpath normalization (os.path mangles "//" and "\\" on Windows); genuine local paths still resolve symlinks via realpath + commonpath. Handles all four metadata-dir forms: local, hf:// remote, zip://...::<local>, and zip://...::hf://<remote>. Scheme rejection in `file_name` and the archive loading tests continue to pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Kaif10 · 2026-07-16T08:28:46Z

Thanks, both addressed.

1. dummy/../../../: this is actually already caught — file_name is run through os.path.normpath before the check, so dummy/../../../etc/passwd normalizes to ../../etc/passwd and is rejected by the existing .. guard. I added explicit regression tests (dummy/../../../etc/passwd, subdir/../../../../outside/secret.txt) so it stays that way.

2. General containment check: done as you suggested — I split both downloaded_metadata_dir and the joined reference on :: and validate the first component, since a relative file_name only ever extends that part (the path inside the archive); the trailing container component(s) must stay identical, which also rejects tampering with the archive path. URL components are normalized with posixpath (os.path rewrites /→\ and collapses // on Windows); genuine local paths still go through realpath + commonpath to resolve symlinks. This now runs for all four metadata-dir forms instead of being skipped for URLs:

/path/to/local/metadata_dir
hf://path/to/remote/metadata_dir
zip://metadata_dir::/path/to/local/archive.zip
zip://metadata_dir::hf://path/to/remote/archive.zip

Added a test covering all four (legit relative passes, sibling-dir escape and container tampering rejected), and the existing scheme-rejection and zip-archive loading tests still pass.

(Disclosure: I used AI assistance while preparing this change.)

lhoestq reviewed Jul 14, 2026

View reviewed changes

lhoestq reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix path traversal via metadata file_name in folder-based builders#8325

Fix path traversal via metadata file_name in folder-based builders#8325
Kaif10 wants to merge 3 commits into
huggingface:mainfrom
Kaif10:fix/8324-metadata-path-traversal

Kaif10 commented Jul 12, 2026

Uh oh!

lhoestq left a comment

Uh oh!

lhoestq Jul 14, 2026

Uh oh!

Kaif10 Jul 15, 2026

Uh oh!

lhoestq left a comment

Uh oh!

lhoestq Jul 15, 2026

Uh oh!

lhoestq Jul 15, 2026

Uh oh!

Kaif10 commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Kaif10 commented Jul 12, 2026

Vulnerability

Fix

Tests

Uh oh!

lhoestq left a comment

Choose a reason for hiding this comment

Uh oh!

lhoestq Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Kaif10 Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

lhoestq left a comment

Choose a reason for hiding this comment

Uh oh!

lhoestq Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

lhoestq Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Kaif10 commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants